Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSP-2060-Use_processing_chain #43

Merged
merged 10 commits into from
Nov 18, 2024

Conversation

ioigoume
Copy link
Contributor

@ioigoume ioigoume commented Nov 7, 2024

No description provided.

@ioigoume ioigoume marked this pull request as draft November 7, 2024 17:34
public/login.php Outdated Show resolved Hide resolved
@ioigoume ioigoume requested a review from pradtke November 9, 2024 19:01
public/login.php Show resolved Hide resolved
public/login.php Outdated Show resolved Hide resolved
src/Cas/Factories/ProcessingChainFactory.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@pradtke pradtke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some minor questions, and I think some files aren't needed.

I was able to test with preprod warning module.

composer.json Outdated
"simplesamlphp/xml-common": "^1.16",
"simplesamlphp/xml-soap": "^1.4"
"simplesamlphp/xml-cas": "^v1.3",
"simplesamlphp/xml-common": "^v1.18",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is upgrading the xml-common library needed? I'm asking since the version in the composer.lock for ssp 2.3.2 full is v1.17.2 and our (Cirrus's) general preference is to be able to install modules into the full version without needing to upgrade any packages already in the lock file.

Are upgrading the other packages needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was targeting the ssp 2.4 release this is why i used the v1.18. I will fallback to ssp 2.3.x and thus use v1.17.

Are upgrading the other packages needed?

the xml-cas package introduces the new attributes. We are discussing about this in the other thread.
the xml-soap has a different directory structure and still supports the soap1.1 and soap1.2. This PR still uses soap1.1

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, so if we target xml-soap 1.4 will it still work with xml-soap 1.5? e.g. can we target ssp 2.3 and still have it work with 2.4?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, so if we target xml-soap 1.4 will it still work with xml-soap 1.5? e.g. can we target ssp 2.3 and still have it work with 2.4?

I do not think i understand the question.

Currently i am developing and testing against ssp 2.3.x but the module targets xml-soap 1.5. The login flows works correctly. Also the composer does not complain. Code wise the main change is the namespaces. Previously they were .../SOAP11/... while now they are .../env_200106/....

tests/resources/xml/testValidServiceUrl.xml Show resolved Hide resolved
tests/src/Cas/Protocol/SamlValidateTest.php Show resolved Hide resolved
tests/config/jwks-key.pem Outdated Show resolved Hide resolved
tests/config/jwks-cert.pem Outdated Show resolved Hide resolved
public/login.php Show resolved Hide resolved
src/Cas/Factories/ProcessingChainFactory.php Outdated Show resolved Hide resolved
@ioigoume ioigoume requested a review from pradtke November 15, 2024 11:29
@ioigoume ioigoume marked this pull request as ready for review November 15, 2024 11:30
Copy link

codecov bot commented Nov 15, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@ioigoume ioigoume requested a review from tvdijen November 18, 2024 09:01
"live" in the container, allowing you to test and iterate different things.

```bash
# Note: this currently errors on this module requiring a newer version of `simplesamlphp/xml-common` than what is in the base image
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be solved in SSP 2.3.3

@tvdijen
Copy link
Member

tvdijen commented Nov 18, 2024

Ignore the failing documentation-action.. It has been resolved in master.

@ioigoume ioigoume force-pushed the SSP-2060-Use_processing_chain branch from 794e47f to c59c5ce Compare November 18, 2024 09:19
@tvdijen
Copy link
Member

tvdijen commented Nov 18, 2024

@ioigoume
Copy link
Contributor Author

@pradtke pradtke merged commit 81b6b53 into simplesamlphp:master Nov 18, 2024
15 checks passed
@ioigoume ioigoume deleted the SSP-2060-Use_processing_chain branch November 18, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants